Support buffering request body (RequestBodyBufferMiddleware) by WyriHaximus · Pull Request #216 · reactphp/http

WyriHaximus · 2017-09-06T20:03:31Z

Buffer Middleware split off from #213
Assumes to be used with #215

clue

Nice, would love to get this in! My main concern with this PR is lack of documentation, looks like some of this somehow ended up in #215 instead?

clue · 2017-09-07T10:28:32Z

tests/Middleware/BufferTest.php

+    public function testToLargeBody()
+    {
+        $size = $this->iniMaxPostSize() + 1;
+        $stream = new BufferStream($size);


Not sure what's supposed to be tested here? Afaict this should probably use a fixed limit instead?

clue · 2017-09-07T10:31:02Z

src/Middleware/Buffer.php

+
+        if ($size === null) {
+            return new Response(411, array('Content-Type' => 'text/plain'), 'No Content-Length header given');
+        }


Makes perfect sense to me, but we should probably add some documentation and tests for this 👍

For the reference: I've discussed this with @WyriHaximus and I think it's save to say we all agree that it may make sense to also support incoming requests with Transfer-Encoding: chunked in a future version (follow-up PR) 👍 This feature is rarely used in the real world and not something that will be needed for most people (unlike outgoing responses with Transfer-Encoding: chunked which are supported).

clue · 2017-09-07T10:31:23Z

src/Middleware/Buffer.php

+{
+    private $sizeLimit;
+
+    public function __construct($sizeLimit = null)


Could use some documentation about the expected parameter type 👍

clue · 2017-09-07T10:37:43Z

src/Middleware/Buffer.php

+use React\Stream\ReadableStreamInterface;
+use RingCentral\Psr7\BufferStream;
+
+final class Buffer


Not sure about the name, as new Buffer() doesn't really convey its responsibility? How about something like RequestBufferer (which doesn't sound perfect either).

RequestBodyBuffer could work.

+1 for RequestBodyBuffer.

Sounds much better to me, so I won't block this 👍

To be this name still implies this class represents the buffer itself, while it is actually (only) responsible for the buffering process and storing the complete buffer back into the request instance that will be passed upstream.

I guess this class could use some documentation? :-)

WyriHaximus · 2017-09-07T19:37:13Z

@clue yes it ended up in #215 by accident, once that is in I'll update this PR with that bit of documentation 👍

clue · 2017-09-08T15:11:47Z

src/Middleware/RequestBodyBuffer.php

+{
+    private $sizeLimit;
+
+    public function __construct($sizeLimit = null)


Could use some documentation about the expected parameter type 👍

clue · 2017-09-08T15:12:21Z

tests/Middleware/RequestBodyBufferTest.php

+    public function testToLargeBody()
+    {
+        $size = $this->iniMaxPostSize() + 1;
+        $stream = new BufferStream($size);


Not sure what's supposed to be tested here? Afaict this should probably use a fixed limit instead?

Replaced that tests with one specifically for the 413 error

clue · 2017-09-08T15:12:58Z

src/Middleware/RequestBodyBuffer.php

+
+        if ($size === null) {
+            return new Response(411, array('Content-Type' => 'text/plain'), 'No Content-Length header given');
+        }


Makes perfect sense to me, but we should probably add some documentation and tests for this 👍

Added a test for it

And added docs for it

jsor · 2017-09-08T18:43:49Z

src/Middleware/RequestBodyBuffer.php

        }

-        return $size;
+        throw new \UnexpectedValueException('post_max_size value is out of range.');


This will now throw for a size specified in bytes (without a suffix)

Good point, I've reverted it

WyriHaximus · 2017-09-09T21:33:16Z

@jsor @clue updated the PR with your feedback 👍

jsor

Updated the PR title with the correct class name.

WyriHaximus · 2017-09-10T09:48:50Z

Thanks! Ping @clue :P

clue

Nice! 🎉 I've rebased and squashed some of your changes and updated the documentation to describe streaming<->buffered request handling, otherwise looks good to me functionally 👍 If you're okay with my changes, let's get this in!

WyriHaximus

Cool, changes look good 👍

WyriHaximus added easy pick help wanted new feature labels Sep 6, 2017

WyriHaximus added this to the v0.8.0 milestone Sep 6, 2017

WyriHaximus requested review from clue and jsor September 6, 2017 20:03

WyriHaximus mentioned this pull request Sep 6, 2017

Middleware #213

Closed

clue reviewed Sep 7, 2017

View reviewed changes

clue reviewed Sep 8, 2017

View reviewed changes

WyriHaximus mentioned this pull request Sep 8, 2017

Rename Server to StreamingServer #197

Closed

jsor reviewed Sep 8, 2017

View reviewed changes

clue removed the easy pick label Sep 10, 2017

jsor changed the title ~~Buffer Middleware~~ RequestBodyBuffer Middleware Sep 10, 2017

jsor approved these changes Sep 10, 2017

View reviewed changes

WyriHaximus mentioned this pull request Sep 10, 2017

LimitHandlers Middleware #218

Closed

clue force-pushed the middleware-buffer branch from 949b061 to 0bbc802 Compare September 11, 2017 12:13

WyriHaximus and others added 3 commits September 11, 2017 14:19

Buffer Middleware

ae26b1f

Renamed Buffer middleware to RequestBodyBuffer

a68b2a3

Rename RequestBodyBuffer to RequestBodyBufferMiddleware

604d79a

clue force-pushed the middleware-buffer branch from 0bbc802 to 4c4e950 Compare September 11, 2017 17:12

Improve documentation for buffering the request body

c8d66b8

clue force-pushed the middleware-buffer branch from 4c4e950 to c8d66b8 Compare September 11, 2017 17:16

clue approved these changes Sep 11, 2017

View reviewed changes

clue changed the title ~~RequestBodyBuffer Middleware~~ Support buffering request body (RequestBodyBufferMiddleware) Sep 11, 2017

WyriHaximus commented Sep 11, 2017

View reviewed changes

WyriHaximus merged commit abdf36e into reactphp:master Sep 11, 2017

WyriHaximus mentioned this pull request Sep 11, 2017

Create a BufferedServer #198

Closed

This was referenced Sep 12, 2017

Support parsing urlencoded request body (RequestBodyParserMiddleware) #220

Merged

Form Urlencoded Request Body Parser Middleware #222

Closed

Multipart Request Body Parser Middleware #223

Closed

This was referenced Sep 19, 2017

Content length buffered sink #70

Closed

react/promise-stream has moved from dev dependency to normal dependency #224

Merged

clue mentioned this pull request Nov 17, 2017

Respect ini settings for file uploads #250

Closed

Uh oh!

Conversation

WyriHaximus commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Sep 9, 2017

Uh oh!

jsor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Sep 10, 2017

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WyriHaximus commented Sep 6, 2017 •

edited

Loading

jsor left a comment •

edited

Loading